Skip to content

Conversation

@wckzhang
Copy link
Contributor

Signed-off-by: William Zhang [email protected]

@wckzhang
Copy link
Contributor Author

There's probably a better way to do this, don't have much experience writing spec files

# type: bool (0/1)
%{!?install_params_file: %define install_params_file 0}
# type: string (full path of params file)
%{!?params_file: %define params_file openmpi-mca-params.conf}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this default file come from?

Or is it an error to specify install_params_file==1 without also specifying a valid params_file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's an error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should make it fail early, then. Otherwise, it doesn't fail until deep during make install, and the error isn't obvious -- it'll say openmpi-mca-params.conf not found, or something. And that will be uber confusing, because people will think that that file does exist.

Copy link
Contributor Author

@wckzhang wckzhang Apr 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking instead to get rid of "install_params_file" and just have "params_file" and a check to see if the string isn't empty in the make install <-- just haven't had the time to test if I can do this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wckzhang Ping. Any progress, perchance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sorry, I will put this on my TODO list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was having a few issues when testing so I put it off for later and never got back to it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wckzhang Do you think you'll finish this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see it in the short term. If you want to close it, I can always re-open if I see the need in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can close it if you like, or perhaps put it in draft mode.

@lanl-ompi
Copy link
Contributor

Can one of the admins verify this patch?

As the cuda copy code needs to be included into the
datatype converter, it introduced an open-pal dependency
on the common component. To avoid this, we move the
code into util. This also addresses a bug with
--enable-dynamic-dso and --with-cuda.

Signed-off-by: William Zhang <[email protected]>
@wckzhang
Copy link
Contributor Author

wckzhang commented Feb 9, 2022

Crap, pushed to master branch caused this PR to be updated

@bwbarrett
Copy link
Member

AWS isn't really interested in this PR at this point, so closing.

@bwbarrett bwbarrett closed this Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants